test(core): golden wire-format fixtures pin VERSION_V3 in-repo#243
Conversation
Tiny golden ciphertexts (both containers, sealed by the published @e4a/pg-wasm 0.6.1) committed with the key material to unseal them, plus tests pinning the literal PRELUDE/VERSION_V3/preamble constants. A change to consts.rs, the bincode framing, or the header layout now fails `cargo test -p pg-core` immediately in this repo — the same tripwire the e2e harness runs externally (postguard-e2e fixtures-verify), but without needing a sibling checkout. Closes #211.
|
Thanks for the PR! I'm on it — kicking off a review of the wire-format tripwire tests and golden fixtures now, and I'll follow up with a consolidated review (approve / request changes / comment) shortly. |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Golden wire-format tripwire — looks good, one accuracy nit.
Solid Phase-1 artifact: literal-pinned constants, both containers exercised end-to-end (parse + KEM/AEAD/signature), a std-only base64 decoder to avoid a dev-dep, and a testdata README that documents the deliberate-bump path. Test keys for test data are fine. Not blocking merge.
One nit inline: the file-level #![cfg(all(feature = "rust", feature = "stream"))] means plain cargo test -p pg-core (default features = rust only) compiles this file to 0 tests, which contradicts the README/module-doc claim that a format change "fails cargo test -p pg-core immediately". CI is safe (build.yml runs --features test,rust,stream), but a maintainer running plain cargo test -p pg-core locally to check compat gets a false green.
| fn golden_fixtures_carry_the_pinned_preamble() { | ||
| for name in ["stream.bin", "mem.bin"] { | ||
| let ct = testdata(name); | ||
| assert!(ct.len() > PREAMBLE_SIZE, "{name}: too short"); |
There was a problem hiding this comment.
The file-level gate requires the stream feature, but only golden_stream_fixture_unseals needs it — wire_constants_are_pinned needs no feature and golden_mem_fixture_unseals needs only rust (the default). Since stream is not a default feature (default = ["rust"]), plain cargo test -p pg-core runs running 0 tests here, so the README/module-doc claim that a format change "fails cargo test -p pg-core immediately" is only true with the stream feature.
Two ways to make the claim hold: gate the file on #![cfg(feature = "rust")] and put #[cfg(feature = "stream")] on just the stream test — or, if the stream-feature requirement is intended, tweak the README/doc wording to say the tripwire needs --features stream. Non-blocking.
|
Reviewed and posted a comment review on #243 — solid PR overall, CI-green, closes #211 as intended. Golden fixtures cover both containers end-to-end (parse + KEM/AEAD/signature verify), constants are correctly pinned, and the testdata README nicely documents the deliberate-bump path. One non-blocking nit: |
The file-level stream gate meant a plain cargo test -p pg-core ran zero of these tests, contradicting the README's 'fails immediately' claim. Now the file gates on the default rust feature (constants, preamble, and the in-memory unseal always run) and only the streaming-container test requires the stream feature (still covered in CI, which tests with test,rust,stream).
|
Fixed, /dobby — went with your first option: the file now gates on the default |
|
Thanks for the fix, @rubenhensen! I'm on it — kicking off a review of the PR now to check correctness and rule-compliance, then I'll submit one consolidated review with a clear verdict. |
There was a problem hiding this comment.
Approving — the feature-gating rework is correct and does what you described.
Verified against HEAD (776f3ba):
- Module gates on the default
rustfeature (#![cfg(feature = "rust")]), sowire_constants_are_pinned,golden_fixtures_carry_the_pinned_preamble, andgolden_mem_fixture_unsealsall run under a plaincargo test -p pg-core. - Only
golden_stream_fixture_unsealsadditionally carries#[cfg(feature = "stream")], with theUnsealerStreamConfigimport moved into the function — clean. CI'stest,rust,streamset covers all four. - The README's "fails immediately" claim now holds for a default-features run.
One non-blocking style note inline (the hand-rolled base64 decoder). Not a bug — the decoder is proven correct by golden_mem_fixture_unseals. Good, focused tripwire PR.
|
|
||
| /// Minimal std-only base64 decoder (standard alphabet, padded) — avoids a | ||
| /// dev-dependency for one meta field. | ||
| fn b64(s: &str) -> Vec<u8> { |
There was a problem hiding this comment.
Small accuracy nit, non-blocking: base64ct is already a regular (non-dev, non-optional) dependency of pg-core (Cargo.toml line 25, used for constant-time base64 in artifacts.rs), so the "avoids a dev-dependency" justification here is not quite right — nothing would be added to the dep tree.
That said, the swap is not entirely free: artifacts.rs uses the buffer-based Base64::decode(v, buf) and base64ct is pulled in without the alloc feature, so Base64::decode_vec would need features = ["alloc"] enabled (or decode into a sized buffer). Given the decoder is tiny and proven correct by the unseal test, this is genuinely optional — feel free to leave it. If you keep it, consider tweaking the comment so it does not claim base64ct is absent.
|
Nice, that lines up exactly with what I verified against HEAD |
1 participant
Closes #211 — the last Phase 1 artifact of EPIC #201.
Puts the wire-format tripwire inside this repo: the e2e harness already guards format drift externally (postguard-e2e
fixtures-verify, proven red on aVERSION_V3bump), but that requires the harness + a sibling checkout. This PR makescargo test -p pg-coreitself fail the moment the format changes.pg-core/testdata/wire-format-v3/— tiny golden ciphertexts in both containers (streaming = cryptify/upload, in-memory =toBytes()), sealed by the published@e4a/pg-wasm@0.6.1, committed with the vk/usk needed to unseal them forever (test keys for test data).pg-core/tests/wire_format.rs— four tests:wire_constants_are_pinned: the literal bytes —PRELUDE == [0x14, 0x8A, 0x8E, 0xA7],VERSION_V3 == 2, 10-byte preamble. Literals on purpose: the constants are the compatibility contract.golden_fixtures_carry_the_pinned_preamble: both containers start withPRELUDE || VERSION_V3 (BE) || header-len.golden_mem_fixture_unseals/golden_stream_fixture_unseals: full parse (bincode header framing) + decrypt (KEM + AEAD + signature verify) of both containers.A red here is always a decision: fix the accidental break, or ship a deliberate new version — regenerating fixtures for the new format via the harness generator while keeping the old set decodable (old ciphertexts don't disappear from inboxes). Documented in the testdata README.
Full pg-core suite green (57 + 4 + 3).